States and projections #1
Conversation
Almaju
force-pushed
the
states-and-projections
branch
from
4c56f37 to
fa3ec8a
Compare
|
@CodiumAI-Agent /review |
PR Analysis
PR Feedback💡 General suggestions: The PR is well-structured and introduces several improvements to the codebase. The use of newtypes for task names and indices improves code readability and error handling. The changes to the command and query handling also make the code more organized and easier to follow. However, it would be beneficial to provide more context and explanation in the PR description for such a large set of changes. This would help reviewers understand the rationale behind each change and how they all fit together. 🤖 Code feedback:
✨ Usage tips:
|
|
@CodiumAI-Agent /improve --extended |
| @@ -0,0 +1,55 @@ | |||
| use crate::port::{TodoListRepository, TodoListStore}; | |||
| use domain::todolist_message::TodoListMessage; | |||
| use framework::*; | |||
There was a problem hiding this comment.
Suggestion: Consider using a more specific import for the framework::* module instead of importing all its contents. [best practice]
| use framework::*; | |
| use framework::{AnyResult, Error}; |
| pub use crate::projections::TodoListProjection; | ||
| pub use domain::{todolist::TodoList, todolist_event::TodoListEvent}; | ||
| pub use crate::projection::TodoListProjection; | ||
| pub use domain::{todolist_event::TodoListEvent, todolist_state::TodoList}; | ||
| use framework::*; |
There was a problem hiding this comment.
Suggestion: Consider using a more specific import for the framework::* module instead of importing all its contents. [best practice]
| use framework::*; | |
| use framework::{AnyResult, Error}; |
| pub enum Command { | ||
| AddTask { name: String }, | ||
| RemoveTask { index: usize }, | ||
| CompleteTask { index: usize }, | ||
| } |
There was a problem hiding this comment.
Suggestion: Consider adding a docstring to the Command enum. [best practice]
| pub enum Command { | |
| AddTask { name: String }, | |
| RemoveTask { index: usize }, | |
| CompleteTask { index: usize }, | |
| } | |
| #[derive(Deserialize)] | |
| /// Represents a command to be executed on the todo list. | |
| pub enum Command { |
| #[derive(Debug, Error)] | ||
| pub enum CommandError { | ||
| #[error("Task name cannot be empty")] | ||
| TaskNameCannotBeEmpty, | ||
| } |
There was a problem hiding this comment.
Suggestion: Consider adding a docstring to the CommandError enum. [best practice]
| #[derive(Debug, Error)] | |
| pub enum CommandError { | |
| #[error("Task name cannot be empty")] | |
| TaskNameCannotBeEmpty, | |
| } | |
| #[derive(Debug, Error)] | |
| /// Represents an error that can occur while executing a command on the todo list. | |
| pub enum CommandError { |
| impl TryInto<TodoListMessage> for &Command { | ||
| type Error = CommandError; | ||
|
|
||
| fn try_into(self) -> Result<TodoListMessage, Self::Error> { | ||
| Ok(match self { | ||
| Command::AddTask { name } => TodoListMessage::AddTask( | ||
| name.clone() | ||
| .try_into() | ||
| .map_err(|_| CommandError::TaskNameCannotBeEmpty)?, | ||
| ), | ||
| Command::RemoveTask { index } => TodoListMessage::RemoveTask((*index).into()), | ||
| Command::CompleteTask { index } => TodoListMessage::CompleteTask((*index).into()), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Consider adding a docstring to the try_into function. [best practice]
| impl TryInto<TodoListMessage> for &Command { | |
| type Error = CommandError; | |
| fn try_into(self) -> Result<TodoListMessage, Self::Error> { | |
| Ok(match self { | |
| Command::AddTask { name } => TodoListMessage::AddTask( | |
| name.clone() | |
| .try_into() | |
| .map_err(|_| CommandError::TaskNameCannotBeEmpty)?, | |
| ), | |
| Command::RemoveTask { index } => TodoListMessage::RemoveTask((*index).into()), | |
| Command::CompleteTask { index } => TodoListMessage::CompleteTask((*index).into()), | |
| }) | |
| } | |
| } | |
| impl TryInto<TodoListMessage> for &Command { | |
| /// Tries to convert the command into a `TodoListMessage`. | |
| /// Returns an error if the conversion fails. | |
| type Error = CommandError; | |
| fn try_into(self) -> Result<TodoListMessage, Self::Error> { | |
| ... |
| impl<R> Execute<R> for Command | ||
| where | ||
| R: TodoListRepository + TodoListStore + Send + Sync, | ||
| { | ||
| async fn execute(&self, runtime: &R) -> AnyResult<()> { | ||
| let message = self.try_into()?; | ||
|
|
||
| // Pull the current state and apply the message | ||
| let todolist = TodoListStore::pull(runtime).await?; | ||
| let new_events = todolist.send(&message)?; | ||
| TodoListStore::push(runtime, &new_events).await?; | ||
|
|
||
| // Save the projection | ||
| let mut projection = TodoListRepository::fetch(runtime).await?; | ||
| projection.apply(&new_events); | ||
| TodoListRepository::save(runtime, &projection).await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Suggestion: Consider adding a docstring to the execute function. [best practice]
| impl<R> Execute<R> for Command | |
| where | |
| R: TodoListRepository + TodoListStore + Send + Sync, | |
| { | |
| async fn execute(&self, runtime: &R) -> AnyResult<()> { | |
| let message = self.try_into()?; | |
| // Pull the current state and apply the message | |
| let todolist = TodoListStore::pull(runtime).await?; | |
| let new_events = todolist.send(&message)?; | |
| TodoListStore::push(runtime, &new_events).await?; | |
| // Save the projection | |
| let mut projection = TodoListRepository::fetch(runtime).await?; | |
| projection.apply(&new_events); | |
| TodoListRepository::save(runtime, &projection).await?; | |
| Ok(()) | |
| } | |
| #[async_trait] | |
| impl<R> Execute<R> for Command | |
| where | |
| R: TodoListRepository + TodoListStore + Send + Sync, | |
| { | |
| /// Executes the command on the todo list. | |
| /// Returns an error if the execution fails. | |
| async fn execute(&self, runtime: &R) -> AnyResult<()> { | |
| ... |
| #[derive(Clone, Default, Debug, Serialize, Deserialize)] | ||
| pub struct TodoListProjection { | ||
| pub in_progress: HashMap<usize, String>, | ||
| pub completed: HashMap<usize, String>, | ||
| } |
There was a problem hiding this comment.
Suggestion: Consider adding a docstring to the TodoListProjection struct. [best practice]
| #[derive(Clone, Default, Debug, Serialize, Deserialize)] | |
| pub struct TodoListProjection { | |
| pub in_progress: HashMap<usize, String>, | |
| pub completed: HashMap<usize, String>, | |
| } | |
| #[derive(Clone, Default, Debug, Serialize, Deserialize)] | |
| /// Represents the projection of the todo list. | |
| pub struct TodoListProjection { | |
| pub in_progress: HashMap<usize, String>, | |
| pub completed: HashMap<usize, String>, | |
| } |
Général :
just testpour pouvoir voir lesprintlnDomain:
sendetapplydans leStatepuisque sinon ça serait contradictoire avec la logique desProjectiontodolist.rsentodolist_state.rspour plus de clartéTaskNameetTaskIndexpour plus de clarté et valider que la string est non-videHashMapplutôt queVecpour mieux gérer les index + erreurs quand l'index n'existe pasApplication :
State -> Projection=> Projection + Event[] -> Projectionpull_and_pushvia unenum CommandCommandetQueryen un seul traitExecute(Effectaurait aussi pu être utilisé ?)Command -> Result<Message, CommandError>Client Axum :
handle_commandgrâce à l'enum/todolistavec GET et POST pour tout gérerNext? :
QueryetProjectioncomme pourCommand?State -> Apply + SendetProjection -> Applypour éviter les collisions et avoir plus de flexibilité ?